-
Notifications
You must be signed in to change notification settings - Fork 9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement basic component API #14
Conversation
@@ -2,12 +2,23 @@ | |||
// These will *not* be published as part of your addon, so be careful that your published code does not rely on them! | |||
|
|||
import '@glint/environment-ember-loose'; | |||
import type HeadlessFormRegistry from '../src/template-registry'; | |||
import { ComponentLike } from '@glint/template'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import { ComponentLike } from '@glint/template'; | |
import type { ComponentLike } from '@glint/template'; |
surprised linting didn't catch this 🤔
or maybe it doesn't matter since in a .d.ts file, everything is a type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I think I don't have your eslint-config for the addon (just the blueprint defaults here), only for the test-app (due to template-imports). I can add it here as well...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added @nullvoxpopuli/eslint-configs
also for the addon now, and indeed --fix
did a couple of changes!
overrides: [ | ||
{ | ||
files: '*.{js,ts}', | ||
files: '*.gjs', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this not automatic? 🤔
maybe I missed something the the README for this plugin
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, probably this shouldn't be there. But this was the only way to make the prettier VSCode plugin work, see ember-tooling/prettier-plugin-ember-template-tag#38
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
making editors work is pretty important, I'm in favor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am adding a comment there to point to that issue, so we can revert that should that get fixed.
@@ -62,12 +62,11 @@ jobs: | |||
fail-fast: false | |||
matrix: | |||
try-scenario: | |||
- ember-lts-3.28 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question, just curious: We aren't supporting 3.28 anymore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, this is the very start of everything, so we never supported anything before! 😆
But you are right, we are starting by not supporting Ember 3.28, at least that's the outcome of some back and forth between Preston and me. But we can revisit this decision, if you think there is a good reason to still support Ember 3.28.
Having Ember 3.28 support is good for existing addons, as that supports apps that still have trouble upgrading to 4. And it's quite unfortunate if addons don't have this overlap, and maybe only the newer version of an addon supports Ember 4, but not Ember 3.28. In this way users are forces to upgrade Ember and the addon at the same time.
But none of this applies here, as this is a green field project, and we don't have any existing Ember 3.28 users, so we can just support what is reasonable for us I think...
@@ -0,0 +1,16 @@ | |||
{{#let (unique-id) (fn @set @name) as |uuid setValue|}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: how come these field
files are just index
files inside of the field folder?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what you mean here with index
files?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a great initial implementation. We're still working out some details on Slack, but I see no reason to hold up this PR. nice work!
|
||
assert | ||
.dom('form > [data-test-user-content]') | ||
.exists('does not render anything on its own'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Super tiny nit, non-blocking: I am not sure how this shows how HeadlessForm
does not render it on it's own. Maybe the message is just a bit confusing to me and can be reworded?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I see how this can be a bit confusing. The selector basically tests that there is no DOM node sitting between <form>
and our <div data-test-user-content>
, i.e. the field
component does not add any markup on its own (which we want to do as little as possible, given were are "headless")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see! Maybe it can be field component contains no markup itself
or something then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's a much better phrase, applied that. TY!
); | ||
|
||
assert.true( | ||
submitHandler.calledWith({ firstName: 'Nicole', lastName: 'Chung' }), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey that's me! 😁
|
||
assert.dom('input').hasAttribute( | ||
'id', | ||
// copied from https://ihateregex.io/expr/uuid/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TIL
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! I left a few tiny comments / questions but more out of curiosity
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Echoing @NullVoxPopuli's statement, this is an excellent step forward! Thanks a bunch!
module('data', function () { | ||
module('data down', function () { | ||
test('data is passed to form controls', async function (assert) { | ||
const data = { firstName: 'Tony', lastName: 'Ward' }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm forever now part of ember-headless-forms! 🎉
Support the most basic component API:
<HeadlessForm>
as the only public componentfield
, which in turn yieldslabel
andinput
components, as well asvalue
andsetValue
for custom markup@data
, passed all the way down toinput
@onSubmit
Closes #3